Skip to content

chore: allow running and linting docs notebooks#493

Merged
mckornfield merged 3 commits into
mainfrom
docs-allow-running-linting/mck
Jun 29, 2026
Merged

chore: allow running and linting docs notebooks#493
mckornfield merged 3 commits into
mainfrom
docs-allow-running-linting/mck

Conversation

@mckornfield

@mckornfield mckornfield commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features
    • Added make docs-check-python-snippets to syntax- and optionally type-check Python fenced-code snippets in Markdown/MDX.
    • Added make docs-run-notebook to execute Fern documentation notebooks with marker-based selection.
    • Enhanced virtualenv bootstrapping with an automatic “Next steps” activation reminder.
  • Documentation
    • Updated docs/command guides to reflect snippet-based linting, new skip markers, and the notebook runner path.
  • Tests
    • Expanded pytest coverage for snippet extraction/linting and notebook/MDX resolution/materialization.
  • Chores
    • Adjusted pytest discovery to include documentation subdirectories; ignore generated *.tmp.md files.

@mckornfield mckornfield requested review from a team as code owners June 26, 2026 22:04
@github-actions github-actions Bot added the chore label Jun 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds bootstrap reminders, docs helper targets, a Python snippet linter with tests, and a Fern notebook runner with tests and usage docs.

Changes

Documentation tooling

Layer / File(s) Summary
Bootstrap activation reminder
Makefile
Adds the expected virtualenv reminder snippet and invokes it from bootstrap targets.
Docs targets and guides
Makefile, docs/AGENTS.md, .claude/commands/docs-lint.md, .claude/commands/docs-test.md
Adds Make targets for snippet linting and notebook execution and updates the matching command guides.
Python snippet linting
docs/_scripts/lint_python_snippets.py
Implements Markdown/MDX snippet discovery, extraction, syntax checking, optional type checking, result display, and CLI handling.
Snippet lint tests
docs/_scripts/test_lint_python_snippets.py, pytest.ini
Adds tests for doc discovery, snippet extraction, skip markers, syntax diagnostics, IPython magics, type-check mapping, and pytest discovery.
Notebook runner and docs
docs/fern/scripts/run_notebooks.py, docs/fern/scripts/README.md, docs/.gitignore, Makefile
Implements Fern MDX and notebook resolution, marker-based selection, notebook execution, cleanup, CLI handling, and runner usage docs.
Notebook runner tests
docs/fern/scripts/test_run_notebooks.py
Adds tests for notebook resolution, selection rules, MDX materialization, and temp file cleanup.

Sequence Diagram(s)

sequenceDiagram
  participant Makefile
  participant Lint as lint_python_snippets.py
  participant Ast as ast.parse
  participant Ty as ty
  Makefile->>Lint: uv run --frozen python docs/_scripts/lint_python_snippets.py DOCS_PATH
  Lint->>Ast: parse fenced python snippets
  Lint->>Ty: uv run --frozen ty check
  Ty-->>Lint: diagnostics
  Lint-->>Makefile: exit code
Loading
sequenceDiagram
  participant Makefile
  participant Runner as run_notebooks.py
  participant Notebook as NotebookSelection
  participant Executor as nmp.testing.notebooks.execute_notebook
  Makefile->>Runner: uv run --frozen python docs/fern/scripts/run_notebooks.py $(ARGS) $(DOCS_PATH)
  Runner->>Runner: select_notebooks(paths)
  Runner->>Notebook: resolve notebook/source pairs
  Runner->>Executor: execute selected notebook
  Executor-->>Runner: result
  Runner-->>Makefile: exit code
Loading

Suggested reviewers

  • maxdubrinsky
  • svvarom
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.96% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: enabling docs notebook running and linting workflows.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs-allow-running-linting/mck

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/_scripts/lint_python_snippets.py`:
- Around line 260-263: The temp file naming in PreparedTypeCheckFile creation is
vulnerable to collisions because the sanitized doc_path-based name in the
temp_path construction can overlap for different documents, and prefix-based
matching can misroute diagnostics when temp paths share prefixes. Update the
path generation and lookup logic around PreparedTypeCheckFile, temp_path, and
the diagnostic mapping code in the affected snippet to use a collision-proof
unique identifier per document and exact temp_path equality when resolving
diagnostics, so each source document maps to only its own temporary file and
line mapping.

In `@docs/AGENTS.md`:
- Around line 15-16: The Makefile examples in the docs use the wrong variable
name, so update the documented invocations for docs-check-python-snippets and
docs-run-notebook to use DOCS_PATH instead of DOC_PATH. Keep the wording aligned
with the actual Makefile contract and ensure the examples match the target names
so users can copy them without hitting the empty-variable guard.

In `@docs/fern/scripts/README.md`:
- Around line 84-86: The README examples for make docs-run-notebook use the
wrong variable name, so the commands won’t pick up the notebook path. Update
both example invocations in README to use DOCS_PATH, matching the
docs-run-notebook target and the variable referenced by Makefile; use the
docs-run-notebook command examples as the anchor for the fix.

In `@docs/fern/scripts/run_notebooks.py`:
- Around line 247-250: The MDX preprocessing in run_notebooks() happens before
the per-notebook failure handling, so a materialize_mdx_as_markdown() error can
stop the whole batch. Move the .mdx conversion inside the existing try block for
each notebook selection so include expansion or file I/O failures are caught,
the notebook is marked failed, and processing continues for the remaining items.
Use the existing run_notebooks flow and the selection.path / run_path handling
to keep the logic localized.
- Around line 33-36: Both notebook URL regexes are too strict because they only
allow a single path segment after blob/, so branch refs with slashes do not
match. Update COLAB_NOTEBOOK_RE and FERN_NOTEBOOK_RE in run_notebooks.py to
accept multi-segment refs before the docs/... notebook path, while still
capturing the notebook path in the existing named path group. Keep the rest of
the matching behavior unchanged so notebook links from .mdx pages continue to
resolve.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 00a5b4ad-15c8-4326-902a-b5046d93d3e0

📥 Commits

Reviewing files that changed from the base of the PR and between 3bb3d92 and ed535ab.

📒 Files selected for processing (7)
  • Makefile
  • docs/AGENTS.md
  • docs/_scripts/lint_python_snippets.py
  • docs/_scripts/test_lint_python_snippets.py
  • docs/fern/scripts/README.md
  • docs/fern/scripts/run_notebooks.py
  • docs/fern/scripts/test_run_notebooks.py

Comment thread docs/_scripts/lint_python_snippets.py Outdated
Comment thread docs/AGENTS.md Outdated
Comment thread docs/fern/scripts/README.md Outdated
Comment thread docs/fern/scripts/run_notebooks.py Outdated
Comment thread docs/fern/scripts/run_notebooks.py
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 21917/28725 76.3% 61.0%
Integration Tests 12582/27405 45.9% 19.2%

@tylersbray tylersbray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but do the code rabbit stuff please.

Comment thread docs/AGENTS.md Outdated
Comment thread docs/fern/scripts/README.md Outdated

@benmccown benmccown left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, my only suggestion from chatting with claude is that we consider adding *.tmp.md to .gitignore in case a SIGKILL interrupts while files are presently copied, to avoid accidentally committing them. Not a big deal though, pretty unlikely and I'd hope humans would catch it.

Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
@mckornfield mckornfield force-pushed the docs-allow-running-linting/mck branch from 276228b to d61e5b4 Compare June 29, 2026 16:25

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/_scripts/lint_python_snippets.py`:
- Around line 355-359: The failure handling in lint_python_snippets.py currently
only surfaces non-zero ty/uv exits when unmatched_lines exists, so silent
failures can be missed. Update the result path around the completed.returncode
check to treat any non-zero exit as a failure even when results is empty, using
the existing completed, results, unmatched_lines, and snippets_by_path flow. If
there are no collected messages, return a shared failure message or equivalent
non-empty output for all affected paths so ty/uv errors cannot pass silently.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2ff08f86-79c3-4816-88b1-989850432cd2

📥 Commits

Reviewing files that changed from the base of the PR and between 276228b and d61e5b4.

📒 Files selected for processing (11)
  • .claude/commands/docs-lint.md
  • .claude/commands/docs-test.md
  • Makefile
  • docs/.gitignore
  • docs/AGENTS.md
  • docs/_scripts/lint_python_snippets.py
  • docs/_scripts/test_lint_python_snippets.py
  • docs/fern/scripts/README.md
  • docs/fern/scripts/run_notebooks.py
  • docs/fern/scripts/test_run_notebooks.py
  • pytest.ini
✅ Files skipped from review due to trivial changes (6)
  • docs/.gitignore
  • pytest.ini
  • docs/AGENTS.md
  • docs/fern/scripts/README.md
  • .claude/commands/docs-test.md
  • .claude/commands/docs-lint.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • Makefile
  • docs/fern/scripts/run_notebooks.py

Comment thread docs/_scripts/lint_python_snippets.py
@mckornfield mckornfield added this pull request to the merge queue Jun 29, 2026
Merged via the queue into main with commit c6e3e84 Jun 29, 2026
56 checks passed
@mckornfield mckornfield deleted the docs-allow-running-linting/mck branch June 29, 2026 17:15
mmogallapalli pushed a commit that referenced this pull request Jun 29, 2026
* chore: allow running and linting docs notebooks

Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>

* chore: address CR

Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>

* chore: more CR address

Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>

---------

Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants